Skip to content

Conversation

@hansthen
Copy link
Collaborator

@hansthen hansthen commented Apr 18, 2025

Close #1989

@hansthen hansthen requested a review from Conengmo April 18, 2025 07:47
@hansthen hansthen changed the title Special case null values in popup and tooltip Issue 1989: Convert null values to empty strings in popup and tooltip Apr 19, 2025
@hansthen hansthen requested a review from ocefpaf April 19, 2025 05:32
@hansthen
Copy link
Collaborator Author

@ocefpaf can you have a look?

@ocefpaf
Copy link
Member

ocefpaf commented Apr 22, 2025

Can we add a test to avoid regression? Maybe ask in #1989 for some code/file we can use?

@hansthen
Copy link
Collaborator Author

Can we add a test to avoid regression? Maybe ask in #1989 for some code/file we can use?

I wrote a small program to visually test this. But since this is behavior in the generated code, it is difficult to test with our current tests frameworks.

I have a few ideas on how to make visual snapshots and compare against those during tests. Is it okay if I work on snapshot / regression testing in a new branch?

@ocefpaf
Copy link
Member

ocefpaf commented Apr 22, 2025

I have a few ideas on how to make visual snapshots and compare against those during tests.

We can use https://github.com/matplotlib/pytest-mpl. I had some success with it in the past. However, we can try a text based text that checks if the HTML has null in those fields.

Is it okay if I work on snapshot / regression testing in a new branch?

Sure. IMO it is OK to merge as-is and we can open an issue for further testing here.

@hansthen
Copy link
Collaborator Author

However, we can try a text based text that checks if the HTML has null in those fields.

It's not so easy. The null values are still there. We just filter them in javascript when we generate the popup.

    let handleObject = feature => {
        if (feature === null) {
            return '';
        } else if (typeof(feature)=='object') {
            return JSON.stringify(feature);
        } else {
            return feature;
        }
    }

I can of course create to test that specific string, but that would feel a bit pointless.

@hansthen
Copy link
Collaborator Author

I created an issue for the regression tests. #2140

@ocefpaf
Copy link
Member

ocefpaf commented Apr 22, 2025

I can of course create to test that specific string, but that would feel a bit pointless.

Indeed. Let's merge and focus on the image comparison test in the future.

@hansthen hansthen merged commit 1043da8 into python-visualization:main Apr 22, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Geojsonpopup change when cell is empty

2 participants